Fix slot-migration-max-failover-repl-bytes unable to accept -1#3443
Conversation
In valkey.conf, slot-migration-max-failover-repl-bytes allows setting to -1 to disable the limit. ``` Setting this to -1 will disable this limit ``` But slot-migration-max-failover-repl-bytes is defined as MEMORY_CONFIG and memtoull() rejects negative inputs, making it impossible to set the value to -1 via config file or CONFIG SET. ``` >>> 'slot-migration-max-failover-repl-bytes "-1"' argument must be a memory value ``` Introduce SIGNED_MEMORY_CONFIG flag for memory configs that also accept plain negative number. When memtoull() fails and this flag is set, fall back to string2ll() for parsing. Use ll2string() for CONFIG GET and rewriteConfigNumericalOption() for CONFIG REWRITE when the value is negative. Add a serverAssert in initConfigValues() to enforce that PERCENT_CONFIG and SIGNED_MEMORY_CONFIG are never combined on the same config, since both use negative values with different semantics. This means we have had this issue since it was introduced in valkey-io#1949. Signed-off-by: Binbin <binloveplay1314@qq.com>
murphyjacob4
left a comment
There was a problem hiding this comment.
Makes sense! Thanks for the fix
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #3443 +/- ##
============================================
- Coverage 76.52% 76.52% -0.01%
============================================
Files 157 157
Lines 79025 79035 +10
============================================
+ Hits 60477 60479 +2
- Misses 18548 18556 +8
🚀 New features to boost your workflow:
|
|
Great! So was |
Signed-off-by: Binbin <binloveplay1314@qq.com>
Yes, i added a test, this require a log change. |
|
The failing test case "Main db not affected when fail to diskless load" has this pattern of writing many commands in a pipeline and reading the replies only afterwards: I have fixed similar tests before using CLIENT REPLY OFF (#3430, #2483). I think we need the same fix here. [EDIT] Fix is here: #3452 |
zuiderkwast
left a comment
There was a problem hiding this comment.
Looks good. I found some minor ideas but nothing blocking.
I didn't review the tests very much.
Signed-off-by: Binbin <binloveplay1314@qq.com>
Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech> Signed-off-by: Binbin <binloveplay1314@qq.com>
…igs (#3440) rewriteConfigFormatMemory() and rewriteConfigBytesOption() used long long parameters to represent memory bytes, but configs like maxmemory are stored as unsigned long long and allow values up to ULLONG_MAX. When the value exceeds LLONG_MAX (e.g. 9223372036854775808), it overflows to negative when passed as long long, causing config rewrite to produce incorrect output like "maxmemory -8589934592gb". Change both functions to use unsigned long long, matching the actual semantics of memory byte values. This is consistent with how numericConfigGet() already handles MEMORY_CONFIG using ull2string(). There are some MEMORY_CONFIG are signed and can be negative: 1. maxmemory-clients is a PERCENT_CONFIG 2. slot-migration-max-failover-repl-bytes is a SIGNED_MEMORY_CONFIG (after #3443) None of them were affected: ``` static void numericConfigRewrite(standardConfig *config, const char *name, struct rewriteConfigState *state) { ... if (config->data.numeric.flags & PERCENT_CONFIG && value < 0) { rewriteConfigPercentOption(state, name, -value, config->data.numeric.default_value); } else if (config->data.numeric.flags & SIGNED_MEMORY_CONFIG && value < 0) { rewriteConfigNumericalOption(state, name, value, config->data.numeric.default_value); } else if (config->data.numeric.flags & MEMORY_CONFIG) { rewriteConfigBytesOption(state, name, value, config->data.numeric.default_value); ... ``` Signed-off-by: Binbin <binloveplay1314@qq.com>
…y-io#3443) In valkey.conf, slot-migration-max-failover-repl-bytes allows setting to -1 to disable the limit. ``` Setting this to -1 will disable this limit ``` But slot-migration-max-failover-repl-bytes is defined as MEMORY_CONFIG and memtoull() rejects negative inputs, making it impossible to set the value to -1 via config file or CONFIG SET. ``` >>> 'slot-migration-max-failover-repl-bytes "-1"' argument must be a memory value ``` Introduce SIGNED_MEMORY_CONFIG flag for memory configs that also accept plain negative number. When memtoull() fails and this flag is set, fall back to string2ll() for parsing. Use ll2string() for CONFIG GET and rewriteConfigNumericalOption() for CONFIG REWRITE when the value is negative. Add a serverAssert in initConfigValues() to enforce that PERCENT_CONFIG and SIGNED_MEMORY_CONFIG are never combined on the same config, since both use negative values with different semantics. This means we have had this issue since it was introduced in valkey-io#1949. Signed-off-by: Binbin <binloveplay1314@qq.com> Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
…igs (valkey-io#3440) rewriteConfigFormatMemory() and rewriteConfigBytesOption() used long long parameters to represent memory bytes, but configs like maxmemory are stored as unsigned long long and allow values up to ULLONG_MAX. When the value exceeds LLONG_MAX (e.g. 9223372036854775808), it overflows to negative when passed as long long, causing config rewrite to produce incorrect output like "maxmemory -8589934592gb". Change both functions to use unsigned long long, matching the actual semantics of memory byte values. This is consistent with how numericConfigGet() already handles MEMORY_CONFIG using ull2string(). There are some MEMORY_CONFIG are signed and can be negative: 1. maxmemory-clients is a PERCENT_CONFIG 2. slot-migration-max-failover-repl-bytes is a SIGNED_MEMORY_CONFIG (after valkey-io#3443) None of them were affected: ``` static void numericConfigRewrite(standardConfig *config, const char *name, struct rewriteConfigState *state) { ... if (config->data.numeric.flags & PERCENT_CONFIG && value < 0) { rewriteConfigPercentOption(state, name, -value, config->data.numeric.default_value); } else if (config->data.numeric.flags & SIGNED_MEMORY_CONFIG && value < 0) { rewriteConfigNumericalOption(state, name, value, config->data.numeric.default_value); } else if (config->data.numeric.flags & MEMORY_CONFIG) { rewriteConfigBytesOption(state, name, value, config->data.numeric.default_value); ... ``` Signed-off-by: Binbin <binloveplay1314@qq.com>
In valkey.conf, slot-migration-max-failover-repl-bytes allows setting
to -1 to disable the limit.
But slot-migration-max-failover-repl-bytes is defined as MEMORY_CONFIG
and memtoull() rejects negative inputs, making it impossible to set the
value to -1 via config file or CONFIG SET.
Introduce SIGNED_MEMORY_CONFIG flag for memory configs that also accept
plain negative number. When memtoull() fails and this flag is set, fall
back to string2ll() for parsing. Use ll2string() for CONFIG GET and
rewriteConfigNumericalOption() for CONFIG REWRITE when the value is
negative.
Add a serverAssert in initConfigValues() to enforce that PERCENT_CONFIG
and SIGNED_MEMORY_CONFIG are never combined on the same config, since
both use negative values with different semantics.
This means we have had this issue since it was introduced in #1949.